fix: verify native prebuild fixtures by wanxiankai · Pull Request #426 · callstackincubator/react-native-node-api

wanxiankai · 2026-08-13T03:14:48Z

Summary

parse each Apple framework Info.plist and verify its executable and bundle identifier
add the competing .node fixture to the Babel plugin test
preserve Node.js module resolution precedence when JavaScript and native addon files share a basename

Test plan

pnpm run build
pnpm --filter react-native-node-api test (61 tests passed)
pnpm exec eslint packages/host/src/node/babel-plugin/plugin.ts packages/host/src/node/babel-plugin/plugin.test.ts packages/node-addon-examples/scripts/verify-prebuilds.mts
pnpm run prettier:check

Closes #424

kraenhansen · 2026-08-13T11:02:37Z

Sorry, this needs a rebase now 🙈

wanxiankai · 2026-08-13T13:24:12Z

Rebased onto the latest next and force-pushed as 2d4f74f. I also switched the plist parser to the package-level @expo/plist export. The branch is now conflict-free.

Re-ran:

pnpm run build
pnpm --filter react-native-node-api test (61/61 passed)
targeted ESLint
pnpm run prettier:check

Everything passes locally. The GitHub Actions workflow is currently awaiting maintainer approval.

kraenhansen · 2026-08-16T08:26:06Z

I pushed a few follow-up refinements (moving the .js/.node precedence check into the shared isNodeApiModule utility instead of the Babel plugin's require.resolve() guard, zod-validating the parsed Info.plist, and reusing escapeBundleIdentifier instead of re-deriving the escaping regex). Thanks for finding and fixing the real bug here, @wanxiankai!

Partially generated by Claude Code

kraenhansen

Verified this against next and I think it fully closes #424:

Info.plist gap: verifyFrameworkInfoPlist asserts CFBundleExecutable/CFBundleIdentifier against exactly what writeFrameworkInfoPlist (packages/host/src/node/prebuilds/apple.ts) writes, reusing the same exported escapeBundleIdentifier helper on both sides so the check can't silently drift from the writer.
Missing .node fixture gap: the new my-addon.node fixture in plugin.test.ts genuinely exercises precedence, not just decorates the test. Before the isNodeApiModule guard added in path-utils.ts, adding that fixture alone (without the fix) would make the pre-existing shortcut check (fs.accessSync(modulePath + '.node')) return true first and fail the "does not touch required JS files" assertion — so the fixture and the fix are load-bearing together, confirming the original issue's claim that the old test passed for the wrong reason.

CI is green (Lint + Unit tests on ubuntu/macos/windows all success; the native app/device jobs show skipped, which is expected for a fork PR without the runner secrets/labels), mergeable_state is clean, and it's already got a maintainer approval.

One minor, non-blocking note for a possible follow-up: COLLIDING_SOURCE_EXTENSIONS includes .cjs/.mjs alongside .js/.json, with a comment attributing the list to "extensions Node's own require() resolves before ever trying .node". Node's documented LOAD_AS_FILE algorithm for an extensionless require() only tries .js, .json, then .node — not .mjs/.cjs — and per docs/HOW-IT-WORKS.md the actual runtime resolution here is Metro's bundler anyway, whose default sourceExts likewise don't include mjs/cjs. So a sibling foo.cjs/foo.mjs wouldn't actually shadow foo.node the way the guard assumes. This only makes the plugin more conservative than necessary (it could skip transforming a real addon require()), not a regression of the bug #424 describes, so it doesn't block merging — just a slightly inaccurate rationale comment worth tightening sometime.

Generated by Claude Code

wanxiankai · 2026-08-24T01:56:52Z

Thanks for catching this. I tightened the collision check in a0cc8ae so it now matches Node's extensionless LOAD_AS_FILE order: only .js and .json shadow .node. I also added regression coverage confirming sibling .cjs and .mjs files do not prevent a real addon from being detected.

Validated locally with:

pnpm --filter react-native-node-api test (62 passed)
pnpm run build
ESLint and Prettier on the touched files

kraenhansen · 2026-10-10T07:05:47Z

Review of whether this closes #424 (compared against current next, 535c1c4):

Both gaps are solved and the approach is sound. verifyFrameworkInfoPlist parses each framework Info.plist and checks CFBundleExecutable and CFBundleIdentifier against the library name and escapeBundleIdentifier(...), so it follows the writer. The my-addon.node fixture now makes the "does not touch required JS files" test meaningful. isNodeApiModule now defers to a sibling .js or .json, but only for extensionless paths. An explicit require('./x.node') is unaffected, and the .cjs and .mjs cases are covered by tests.
Blocking: the branch needs a rebase. GitHub reports mergeable_state: dirty, and the PR shows 113 changed files and 27 commits (changesets, workflows, Hermes C++, cmake-rn). They come from an older rewritten next history, not from this change. The real change is about 6 files: path-utils.ts and its test, plugin.test.ts, verify-prebuilds.mts, the index.ts export of escapeBundleIdentifier, and the @expo/plist and zod entries in node-addon-examples/package.json with the lockfile. Please rebase onto the current next so the diff shrinks to those files.
Missing changeset. isNodeApiModule is a user-visible behaviour fix in react-native-node-api, and the new escapeBundleIdentifier export is public API. Add a patch changeset for the package.
Minor. The plist.default workaround for @expo/plist under ESM interop is fragile. Keep its comment, but a createRequire-based import would avoid it.

Generated by Claude Code

kraenhansen · 2026-10-11T21:10:55Z

Rebased onto main and retargeted it there, since this doesn't depend on the Hermes changes on next — the PR now contains just your commits (I regenerated pnpm-lock.yaml to resolve a conflict).

kraenhansen · 2026-10-11T21:35:42Z

Pushed two follow-ups: the Info.plist check now parses the binary plists xcodebuild emits (via bplist-parser, asserting only CFBundleExecutable + the executable) and verifies whichever slices were built, and the iOS CI job now runs verify — which needs #473 (unsigned framework targets) to land first.

- Replace the Babel-transform-time require.resolve() guard with a check inside isNodeApiModule itself, so the fix lives in the shared utility (also used by findNodeAddonForBindings) instead of duplicating Node's module resolution algorithm via a second, independent code path that could diverge from what Metro actually resolves at runtime. - Verify the Info.plist contents with a zod schema instead of ad hoc "in" checks on an untyped object, matching how the rest of the repo validates untrusted structured data. - Reuse the exported escapeBundleIdentifier instead of re-deriving the bundle-identifier escaping regex inline in the verify script, so the two can't silently drift apart. Closes callstackincubator#424 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q1k6UQJPPaqKEKmnsRUatt

xcodebuild emits binary Info.plist files for CMake framework targets, so read them with bplist-parser (falling back to @expo/plist for XML ones). Only assert CFBundleExecutable and that the executable exists, since the identifier and CFBundleName come from CMake rather than the host. Verify whichever slices were built instead of a fixed list, allowing dSYMs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

kraenhansen · 2026-10-11T21:58:30Z

Rebased onto main now that #473 has landed, so the new "Verify prebuilds" step in the iOS job should pass.

kraenhansen force-pushed the next branch from 679e0d4 to c73d30c Compare August 13, 2026 10:15

kraenhansen reviewed Aug 13, 2026

View reviewed changes

Comment thread packages/node-addon-examples/scripts/verify-prebuilds.mts Outdated

kraenhansen self-assigned this Aug 13, 2026

wanxiankai force-pushed the fix/verify-prebuild-fixtures branch from 33ca234 to 2d4f74f Compare August 13, 2026 13:22

wanxiankai requested a review from kraenhansen August 13, 2026 13:24

kraenhansen force-pushed the fix/verify-prebuild-fixtures branch from a848741 to 7e212f5 Compare August 16, 2026 18:22

kraenhansen approved these changes Aug 16, 2026

View reviewed changes

kraenhansen reviewed Aug 21, 2026

View reviewed changes

This was referenced Aug 28, 2026

Verify prebuild metadata and JS addon precedence #455

Closed

Two test gaps: unverified prebuild Info.plist and a missing .node fixture in the Babel plugin tests #424

Open

kraenhansen approved these changes Aug 28, 2026

View reviewed changes

kraenhansen force-pushed the next branch from 0a29fbd to 4bbca65 Compare October 7, 2026 11:03

kraenhansen mentioned this pull request Oct 11, 2026

fix: keep .js/.json precedence over .node and verify prebuild Info.plist #467

Open

kraenhansen force-pushed the next branch from 535c1c4 to 9c6d9ca Compare October 11, 2026 20:30

kraenhansen added Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) labels Oct 11, 2026

kraenhansen changed the base branch from next to main October 11, 2026 21:10

kraenhansen force-pushed the fix/verify-prebuild-fixtures branch from a0cc8ae to ab2dadb Compare October 11, 2026 21:10

This was referenced Oct 11, 2026

Parse binary plists with bplist-parser in readAndParsePlist #472

Open

cmake-rn: honor CODE_SIGNING_ALLOWED for framework targets #473

Merged

wanxiankai and others added 5 commits October 11, 2026 23:57

fix: verify native prebuild fixtures

d964e36

fix: match Node addon resolution extensions

63a903b

ci: verify example prebuilds in the iOS job

b52cc14

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

kraenhansen force-pushed the fix/verify-prebuild-fixtures branch from ea26beb to b52cc14 Compare October 11, 2026 21:58

Navigation Menu

Repository navigation

fix: verify native prebuild fixtures - #426

fix: verify native prebuild fixtures#426
wanxiankai wants to merge 5 commits into
callstackincubator:mainfrom
wanxiankai:fix/verify-prebuild-fixtures

wanxiankai commented Aug 13, 2026

Uh oh!

Uh oh!

kraenhansen commented Aug 13, 2026

Uh oh!

wanxiankai commented Aug 13, 2026

Uh oh!

kraenhansen commented Aug 16, 2026 •

edited

Loading

Uh oh!

kraenhansen left a comment

Uh oh!

wanxiankai commented Aug 24, 2026

Uh oh!

kraenhansen commented Oct 10, 2026

Uh oh!

kraenhansen commented Oct 11, 2026

Uh oh!

kraenhansen commented Oct 11, 2026

Uh oh!

kraenhansen commented Oct 11, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

wanxiankai commented Aug 13, 2026

Summary

Test plan

Uh oh!

Uh oh!

kraenhansen commented Aug 13, 2026

Uh oh!

wanxiankai commented Aug 13, 2026

Uh oh!

kraenhansen commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kraenhansen left a comment

Choose a reason for hiding this comment

Uh oh!

wanxiankai commented Aug 24, 2026

Uh oh!

kraenhansen commented Oct 10, 2026

Uh oh!

kraenhansen commented Oct 11, 2026

Uh oh!

kraenhansen commented Oct 11, 2026

Uh oh!

kraenhansen commented Oct 11, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kraenhansen commented Aug 16, 2026 •

edited

Loading